-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
🐛 pkg/certwatcher: Start should retry for 10s when adding files #2160
🐛 pkg/certwatcher: Start should retry for 10s when adding files #2160
Conversation
/assign @sbueringer |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vincepri The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
pkg/certwatcher/certwatcher.go
Outdated
} | ||
return true, nil | ||
}); err != nil { | ||
return errors.Wrapf(kerrors.NewAggregate([]error{err, watchErr}), "failed to add watches") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the failed job I assume this would be the first direct use of the errors lib in CR. Intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, don't see why not use it, nicer APIs :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wasn't sure given it was never used before in CR. And also it's unmaintained since a while (https://github.com/pkg/errors)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair, fixed
48c0a8e
to
9032bc9
Compare
/lgtm /hold |
This fixes a flake in CI, but it could also come in handy when running the certwatcher against volume mounted certificates. Ideally the timeout is going to be configurable at some point, for now, let's just retry for a fixed number of seconds, before returning an error. Signed-off-by: Vince Prignano <vincepri@redhat.com>
9032bc9
to
ab55747
Compare
Thx! /lgtm |
/hold cancel |
This fixes a flake in CI, but it could also come in handy when running the certwatcher against volume mounted certificates. Ideally the timeout is going to be configurable at some point, for now, let's just retry for a fixed number of seconds, before returning an error.
Signed-off-by: Vince Prignano vincepri@redhat.com